Skip to content

Conversation

@sensei-hacker
Copy link
Member

No description provided.

sensei-hacker and others added 30 commits November 30, 2025 02:34
This commit fixes issues in the transpiler API definitions where
the configurator did not match the actual INAV firmware implementation.

Issues Fixed:

1. waypoint.js (CRITICAL):
   - Fixed wrong operand type (was 5/GVAR, now 7/WAYPOINTS)
   - Removed 6 fabricated properties not in firmware (latitude, longitude,
     altitude, bearing, missionReached, missionValid)
   - Added 14 actual properties from firmware (isWaypointMission, number,
     action, nextAction, distance, distanceFromPrevious, user action flags)

2. gvar.js (HIGH):
   - Fixed wrong operand type (was 3/FLIGHT_MODE, now 5/GVAR)
   - Fixed wrong operation (was 19/GVAR_INC, now 18/GVAR_SET)

3. pid.js (MAJOR):
   - Removed fabricated properties not in firmware (setpoint, measurement,
     P/I/D/FF gains, enabled)
   - Kept only 'output' property which is actually exposed (operands 0-3)
   - Changed from i*10+0..7 mapping to direct i mapping

4. flight.js (MEDIUM):
   - Added missing wind parameters 46-49:
     minGroundSpeed, horizontalWindSpeed, windDirection, relativeWindOffset

5. inav_constants.js (MEDIUM):
   - Added missing FLIGHT_PARAM constants 46-49
   - Added missing OPERATION constant 56 (OVERRIDE_MIN_GROUND_SPEED)

6. codegen.js (LOW):
   - Fixed RC channel regex to support both rc[N] and rc[N].value syntax

Testing:
- Added 5 comprehensive test files (test_rc_channels, test_gvar, test_pid,
  test_waypoint, test_flight)
- All fixes verified against firmware source code:
  - src/main/programming/logic_condition.h
  - src/main/programming/logic_condition.c
  - src/main/programming/global_variables.h
  - src/main/programming/pid.h

Files verified correct (no changes needed):
- override.js (all 10 operations match firmware)
- rc.js (correctly handled by codegen)

Documentation:
- API_BUGS_FOUND.md: Initial issue analysis
- VERIFICATION_SUMMARY.md: Complete verification findings
- FIXES_COMPLETE.md: Final fix summary with cross-references

Breaking Changes:
- pid[N].configure(), pid[N].setpoint, pid[N].enabled no longer available
  (these never existed in firmware, only pid[N].output works)
- waypoint.latitude/longitude/altitude/bearing no longer available
  (these are not exposed through logic condition system)

Firmware References:
- logic_condition.h lines 92-102 (operand types)
- logic_condition.h lines 104-155 (flight parameters)
- logic_condition.h lines 177-192 (waypoint parameters)
- logic_condition.c lines 575-669 (waypoint implementation)
- logic_condition.c lines 1078-1082 (PID implementation)
Updated test documentation to reflect fixed state and improve code quality:

1. test_flight.js:
   - Removed outdated 'KNOWN ISSUE' header (params 46-49 are now present)
   - Updated to use FLIGHT_PARAM_NAMES constant instead of Object.keys().find()
   - Cleaner and more performant parameter name lookup

2. test_pid.js:
   - Replaced 'KNOWN BUG TO DETECT' with 'FIRMWARE DESIGN' explanation
   - Clarified that firmware intentionally exposes only PID outputs
   - Removed 'bug' language as this is intentional firmware design

These changes address feedback from PR review bot.
Fix transpiler API definitions to match firmware implementation
search.js: Update fetch to ESM import syntax
Removed gvar.js and override.js as they are not used by the transpiler.

Analysis shows:
- gvar.js: Completely bypassed by hardcoded gvar handling in codegen.js
  (lines 598-610) and action_generator.js (lines 98, 108, 118, 133, 141)
- override.js: Bypassed by hardcoded operation mapping in codegen.js
  (lines 705-719)

Both files contained only documentation that duplicated information already
hardcoded in the transpiler implementation. Removing them reduces maintenance
burden and eliminates confusion from misleading values (gvar.js had wrong
type and operation values that were never actually used).

Files modified:
- Deleted: js/transpiler/api/definitions/gvar.js
- Deleted: js/transpiler/api/definitions/override.js
- Updated: js/transpiler/api/definitions/index.js (removed imports/exports)
The previous commit removed override.js but forgot to update the import
statement in js/transpiler/index.js, causing build failures:
  "Could not resolve ./api/definitions/override.js"

Changes:
- Removed import of overrideDefinitions from index.js (line 23)
- Removed overrideDefinitions from export list (line 40)

Build verified successful with npm run make.
 JS Programming: Fix tab loading + Monaco editor  import
When saving a JavaScript program to the flight controller, previously-
occupied logic condition slots that are not part of the new transpiled
script are now properly cleared.

Problem:
- User has 20 logic conditions on FC (from previous save)
- User writes JavaScript that transpiles to 10 logic conditions
- Saves to FC
- BUG: FC had conditions 0-9 (new) PLUS 10-19 (old, stale data)

Root Cause:
saveToFC() only sent the new conditions to the FC, leaving old
conditions in their previously-occupied slots.

Solution:
1. Track which slots were occupied when loading from FC
2. When saving, identify slots that were occupied but are NOT in the
   new transpiled script
3. Send disabled/empty conditions for those slots to clear them

Changes:
- tabs/javascript_programming.js (onLogicConditionsLoaded):
  Track previously occupied slots in self.previouslyOccupiedSlots
- tabs/javascript_programming.js (saveToFC):
  Add empty conditions for previously-occupied slots not in new script

Testing:
- Build successful (npm run make)
- Configurator starts without errors
- Manual testing recommended: Create 15 conditions via Programming tab,
  then save 5-condition JavaScript program and verify only 5 remain

This fix prevents stale logic conditions from remaining active on the
flight controller, which could cause unexpected behavior during flight.
Bug #1: Update GPS example to use 'gpsSats' property
- The API property was renamed from 'gpsNumSat' to 'gpsSats'
- GPS Fix Check example was not updated, causing transpilation errors
- Fixed examples/index.js lines 120, 124

Bug #2: Fix waypoint example to use 'distance' property
- Example incorrectly used 'waypoint.distanceToHome'
- Correct property is 'waypoint.distance' (distance to current waypoint)
- Fixed examples/index.js lines 185, 189

Bug #3: Add null checks in property_access_checker.js
- Missing null check caused crash: "Cannot read properties of undefined (reading 'targets')"
- Affected "Altitude-based Stages" and other override examples
- Added defensive checks for apiObj, apiObj.targets, apiObj.nested
- Fixed property_access_checker.js lines 170-181

Fixes:
- "GPS Fix Check" example now transpiles successfully
- "Waypoint Arrival Detection" example now transpiles successfully
- "Altitude-based Stages" example now transpiles successfully
- All 15 examples validated

Users can now successfully use all built-in examples without errors.
The previous implementation used put() to append conditions sequentially,
which didn't correctly map array indices to FC slot numbers. This caused
empty conditions to be sent to the wrong slots when clearing stale data.

Fixed by:
- Building a Map of slot index -> condition from transpiler output
- Adding empty conditions for previously-occupied slots to the map
- Sorting by slot index before adding to FC.LOGIC_CONDITIONS array
- This ensures array position matches FC slot number for sendLogicConditions()

The sendLogicConditions() function uses array index as the FC slot number
(line 2469 in MSPHelper.js), so maintaining correct array indices is critical.

Addresses code review feedback from Qodo bot on PR #2452.
When code uses both direct (<, >, ==) and synthesized (>=, <=, !=)
comparison operators with the same operands, the transpiler was creating
duplicate conditions instead of reusing existing ones.

Example:
  if (x < 6) { ... }   // Creates condition 0: x < 6
  if (x >= 6) { ... }  // Was creating duplicate condition 2: x < 6
                       // Should reuse condition 0

Root cause:
The condition cache had separate namespaces for 'binary' (direct ops)
and 'binary_synth' (synthesized ops). When generating >= as NOT(x < 6),
the code didn't check if 'x < 6' already existed in the binary cache.

Fix:
In generateBinary(), when processing synthesized operators (>=, <=, !=),
check if the inverse comparison already exists in the cache before
generating a new one. If found, reuse the existing LC index.

Changes:
- js/transpiler/transpiler/condition_generator.js: Added cache lookup
  for inverse comparison before generating new condition
- Added comprehensive tests for duplicate detection

Testing:
- test_not_precedence.js: Verifies >= reuses existing <
- test_duplicate_detection_comprehensive.js: Tests >=, <=, != reuse
- All tests pass

Impact: Reduces wasted logic condition slots, prevents FC slot exhaustion
Fixes two issues reported by Scavanger on PR #2455:

1. types.js was referencing apiDefinitions.override which no longer
   exists after override.js was removed in commit 2ed9320
   - Removed line 44: generateInterfaceFromDefinition('override', ...)

2. tabs/javascript_programming.js was using editorWorker() without
   importing it
   - Added missing import on line 21

Both issues were oversights from the API definition cleanup.
…ed-conditions

Fix JavaScript Programming: Clear unused logic conditions on save
Fix three bugs breaking JavaScript Programming examples
…or-precedence

Fix duplicate condition bug in transpiler for synthesized operators
Resolves multiple bugs in the JavaScript Programming tab mentioned in #2453

1. IntelliSense contamination with browser APIs
   - Configured Monaco editor to use ES2020 lib only
   - Excludes DOM/browser APIs (navigator, document, etc.)
   - Preserves Math and standard JavaScript features

2. Override property validation errors
   - Restored override.js API definitions
   - Updated to use OPERATION constants from inav_constants.js
   - Enables proper IntelliSense for override.vtx.power, etc.

3. Unsaved changes warning behavior

Implementation details:
- monaco_loader.js: Added lib: ['es2020'] to compiler options
- override.js: Restored from git history, updated with OPERATION constants
- serial_backend.js: Check isDirty before disconnect, clear flag after confirm
- configurator_main.js: Check isDirty before tab switch
- javascript_programming.js: Simplified cleanup() to only dispose editor

Testing:
- IntelliSense no longer suggests browser APIs
- Override properties validate correctly
- Unsaved changes warning shows exactly once
- Cancel properly prevents disconnect/tab switch
- Subsequent actions work correctly after cancel

Files changed:
- js/transpiler/editor/monaco_loader.js
- js/transpiler/api/definitions/override.js (restored)
- js/transpiler/api/definitions/index.js
- js/transpiler/api/types.js
- js/serial_backend.js
- js/configurator_main.js
- tabs/javascript_programming.js
…g-issues

Fix JavaScript Programming tab issues (#2453)
Implements full compiler and decompiler support for flight axis angle/rate overrides.

Syntax:
  override.flightAxis.{roll|pitch|yaw}.{angle|rate} = value

INAV operations:
- FLIGHT_AXIS_ANGLE_OVERRIDE (45): angle in degrees
- FLIGHT_AXIS_RATE_OVERRIDE (46): rate in deg/s

Changes:
- Added override.js with flightAxis API definitions
- Made TypeScript generation recursive for nested objects
- Encode axis (0=roll, 1=pitch, 2=yaw) in operandA
- Removed decompiler warnings
- Added missing operations: osdLayout, loiterRadius, minGroundSpeed

Testing:
- 130+ tests pass (116 existing + 14 new regression tests)
- No regressions found
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
sensei-hacker and others added 29 commits January 6, 2026 12:15
Address Qodo code review feedback: check result.warnings.errors
instead of result.errors for semantic/validation errors.
Set tpa_rate = 80 for airplane presets ("Airplane with a Tail" and
"Airplane without a Tail"). This addresses feedback from PR #11222
that fixed-wing aircraft benefit from higher TPA values.

Multicopter presets retain their existing tpa_rate = 20 setting.
…y key

Prevents generating error suggestions containing 'undefined' when
nested.properties is an empty object.

Addresses qodo-merge-pro bot suggestion on PR #2514.

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
The plotElevation() function was commented out in commit 8d5870b
due to ESM module compatibility issues. This fix:
- Adds Chart.js v4.4.1 with full ESM support to package.json
- Imports Chart.js properly as ES module
- Uncomments the plotElevation() function

This restores the terrain elevation profile chart feature for
mission planning safety checks.
The plotElevation() function calls `new Chart()` which expects Chart
to be available in the global scope. While we correctly imported
Chart.js as an ES module, it was only available within the module
scope. This change exposes Chart on the window object so the
elevation plotting function can access it.
The plotElevation() function had Chart.js code only for empty missions
(dummy data). When missions had waypoints, it prepared elevation data
but only had a commented-out Plotly.newPlot() call, so no chart
rendered.

Changes:
- Add Chart.js rendering after elevation data preparation (line 4352+)
- Convert Plotly trace data to Chart.js datasets format
- Display WGS84 terrain elevation (orange, filled area)
- Display mission altitude (blue line with waypoint markers)
- Properly destroy existing chart before creating new one
- Store chart instance globally for cleanup on subsequent calls

Both empty and non-empty mission cases now create Chart.js charts.
Code review identified several issues that needed addressing:

CRITICAL fixes:
- Replace hardcoded test data in empty mission case with proper empty chart
- Add canvas element validation before Chart instantiation
- Remove all unused Plotly code structures (trace objects, layout vars)
- Fix chart title to not depend on removed Plotly code

IMPROVEMENTS:
- Add try-catch error handling for async elevation data fetch
- Add safe array checks for Math.min/max to prevent empty array errors
- Change var to const for immutable values (modern JavaScript)
- Clear chart instance reference (set to null) after destruction
- Add console error messages for debugging

The plotElevation() function is now a clean Chart.js-only implementation
without any Plotly remnants, proper error handling, and validation.
…ctivity

When waypoints are dragged, the elevation chart should update smoothly.
The previous approach of destroying and recreating the chart on every
update was causing the chart to not refresh when waypoints were moved.

Chart.js update() method is designed for this use case:
- Preserves the chart instance
- Updates data and options
- Triggers smooth re-render with animations
- More efficient than destroy/recreate

This fixes the issue where dragging a waypoint didn't update the
elevation profile.
…cted

Previously, plotElevation() was only called if the dragged waypoint
matched selectedMarker. This meant dragging an unselected waypoint
wouldn't update the chart.

Now plotElevation() is called for all waypoint drags:
- If waypoint is selected: updates elevation + runs full async logic
- If waypoint is not selected: updates elevation chart only

This ensures the elevation profile always reflects the current mission
geometry after any waypoint is moved.
Per code review feedback, adds two important improvements:

1. Race condition protection:
   - Track elevation update sequence number
   - Ignore stale data from late-returning API calls
   - Prevents chart showing outdated elevation when waypoints dragged rapidly

2. Disable chart animations during updates:
   - Use chart.update('none') instead of chart.update()
   - Improves performance during drag operations
   - Prevents animation queueing and visual lag

These fixes ensure the chart always shows current data and responds
smoothly even with rapid waypoint dragging or slow network connections.
…aries

Fix postPackage hook to remove non-native SITL binaries from macOS builds
Fix transpiler validation and improve error messages
…aults

Add TPA rate defaults to fixed-wing presets
…rt-esm

Fix: Re-enable terrain elevation chart with Chart.js ESM support
- Linux x64 SITL: built locally with glibc 2.34 compatibility
- Linux arm64, macOS, Windows SITL: from nightly v9.0.0-20260114.184
- Firmware commit: a7932b92eadd92ba7851a009a5c974b9cc24835d
- All binaries match 9.0.0 final release candidate
removed old pre 9.0 values I missed
Fix formatting of OSD element document
Update SITL binaries for 9.0.0 final release
In 8.0.0, addOnReceiveListener() called addOnReceiveCallback() which
registered callbacks in both _onReceiveListeners and the BLE-specific
_onCharateristicValueChangedListeners array. In 9.0.0 this was removed
to "avoid duplicate push", but Serial's addOnReceiveCallback() pushes
to _onReceiveListeners while BLE's pushes to its own separate array.

This meant the byte counter listener (added via addOnReceiveListener)
only went into _onReceiveListeners, but BLE's notification handler only
called _onCharateristicValueChangedListeners - so the counter never
incremented and MSP responses were never seen by the parser.

Fix: align BLE with Serial by using _onReceiveListeners throughout,
removing the now-unnecessary _onCharateristicValueChangedListeners array.
Includes debug logging to aid diagnosis if further issues arise.
User confirmed BLE connection is working correctly after the byte
counter fix, but reported slower performance compared to 8.0.1.

Analysis showed 4 console.log() statements in the data path:
- 2 logs on every BLE notification received (hot path)
- 2 logs during listener add/remove (cold path)

The hot path logs were causing performance degradation as they
execute on every 20-byte BLE packet received. Removed all 4
debug logs as they were marked TODO for removal after testing.

The core fix (using _onReceiveListeners array) remains intact.
Fix BLE byte counter regression introduced in 9.0.0
@sensei-hacker sensei-hacker merged commit 49de051 into maintenance-10.x Jan 30, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants